Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC:Remove devtools from main gem set #2421

Closed
wants to merge 7 commits into from
Closed

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Nov 24, 2022

It would be best to have the test matrix run with only gems relevant to the test at hand.

We have many development tools in our Gemfile, which get included in all gem set combinations, in some occasions causing conflicts.

It seems more practical to me, at this point, to separate everything that we know is not necessary to run ddtrace or its test suite into separate "groups", and keep what's left as "essential to ddtrace or testing ddtrace".

This PR, at this point, is just a sketch for possible approaches we can take.

Gemfile Outdated
Comment on lines 76 to 78
group :appraisal do
gem 'appraisal', '~> 2.2'
end
Copy link
Member Author

@marcotc marcotc Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach 1

Very specific groups, pretty much matching the tool name: e.g. appraisal, rubocop, yard, sorbet, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be extremely granural, not too keen on that solution.

Gemfile Outdated
Comment on lines 80 to 84
group :release do
gem 'pimpmychangelog', '>= 0.1.2' # Formats the CHANGELOG.md file
gem 'redcarpet', '~> 3.4' if RUBY_PLATFORM != 'java' # Used by YARD to generate docs
gem 'yard', '~> 0.9'
end
Copy link
Member Author

@marcotc marcotc Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach 2

Locally grouped gems, matching the high-level activity they are part of: e.g. release, static_check, benchmark, test, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach 2a

Locally grouped gems, but more granular then 2: e.g. changelog (has pimpmychangelog), docs (has redcarpet & yard), lint (has rubocop*), type_check (has sorbet & spoom) etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this better than Approach 1.

I like Approach 2 better than Approach 2a as I don't think we need to go that deep.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have strong thoughts either way, but I think coarser-grained groups seem like a good enough start, and we could always break them up as needed later.

Gemfile Outdated
gem 'yard', '~> 0.9'
end

group :static_check do
Copy link
Member

@lloeki lloeki Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually call that just :check or :lint (not the least because the sorbet gem is not static: it evaluates files!)

@lloeki
Copy link
Member

lloeki commented Nov 24, 2022

I'm very much liking this, if only because it should resolve the aarch64 vs x86_64 inconsistency of Sorbet in appraisal gemfiles and lockfiles (which I planned to solve in this way but this goes further)

@ivoanjo
Copy link
Member

ivoanjo commented Dec 9, 2024

This looks like a neat idea; nevertheless, I'm thinking this needs a lot of rework to make useful again -- we've moved A LOT of stuff around since this PR was opened.

So... I'm going to go ahead and close this one until we have bandwidth to restart it >_>

@ivoanjo ivoanjo closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants